-
-
Notifications
You must be signed in to change notification settings - Fork 910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize error message of association matcher #1417
Conversation
9a65ceb
to
eaad2ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had two comments. This error message is not the greatest anyways, IMO I don't know if it always points users to what the problem actually is or what the solution is, but that is not your fault and seems out of scope for your change here, so I'm okay with letting it slide.
Out of curiosity, which test failed on Rails 6.1 and why?
I'm opening this PR, #1418, just to show the errors that are happening with Rails 6.1. One of them is:
Before Rails 6.1 this lines was returning an array of symbols:
But now is returning a string. Example: "[:company_id, :person_detail_id]". The logic on the association matcher checks if this is an array: shoulda-matchers/lib/shoulda/matchers/active_record/association_matcher.rb Lines 1453 to 1458 in 9763c17
This logic was supposed to update that value to "["company_id", "person_detail_id"], but it's not working because it's not returning an array anymore. I think this was the commit that made this impact: rails/rails@c0750fe Standardizing the error message to be symbols solves this error. What do you think? |
eaad2ff
to
f42588d
Compare
f42588d
to
c3917e6
Compare
@vsppedro As long as we don't change what the developer is passing to |
I'll go back to this as soon as I finished dropping rails 4.2, 5.0 and 5.1. |
c3917e6
to
371c107
Compare
371c107
to
d235ed4
Compare
Done! |
d235ed4
to
08fdca6
Compare
08fdca6
to
f0f1801
Compare
I decided to merge this PR into #1418. I think it will be simpler to work with just one branch. |
I noticed that the association matcher was returning a slightly different message between the tests below:
shoulda-matchers/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb
Lines 1451 to 1454 in 9763c17
shoulda-matchers/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb
Lines 1436 to 1439 in 9763c17
Pay attention to the foreign_keys, their types are different. This PR standardized the error message - in both cases the foreign_keys will be symbols. Besides that, these modifications fix one of the four tests that are failing with Rails 6.1.